Repository navigation
feat(arcade): Arcade as a second connected-app provider (PR 1) - #770
Conversation
…live run All of this was measured in the first live run against the Arcade API on this branch, from this deployment's own code. `provider_id` names three different things, and nothing in the tree said so: on a tool listing it is the provider TYPE (`google`), on a user-connection row the provider INSTANCE id (`google-openbot`, `cpk-poc`), and on an auth-provider config the two sit side by side, `id` being the instance and `provider_id` the type. A connection row carries no vendor-type field at all — its `provider_type` is `"oauth2"`, the auth scheme — so the instance id is the only link to the vendor. The `authProviderOf` docblock claimed the member answered with a configuration; it answers with the type, which is what made the comparison fixed in 28b13e4 look right. The operator page gains that distinction with the verbatim config and connection-row shapes, and notes that it bit this deployment once: a connection that existed read as not connected. Moved out of "unproven" on that page, all now measured: - sibling revocation — one `google` provider serves `Gmail` and `GoogleCalendar`, so disconnecting one app can end another, in production; - Google's per-action isolation — 0 of 30 scope-carrying `Gmail` actions authorized before one real consent to `Gmail.GetEmail`, 11 of 30 after, and nothing beyond the granted scopes; - the wrong-user confirm shape — a generic `400 coordinator_error "Bad request"` naming no mismatch, so a wrong-account return presents as the vendor-unavailable refusal rather than its own sentence. Added as unproven: Google's "Use this client for an AI-powered agent" checkbox, a self-declaration that puts the client under a separate admin-policy surface and which Google advises keeping apart from human-workflow clients. OpenBot's brokered calls are agent-initiated, so a deployer should choose deliberately; it was left unticked today to keep a policy variable out of the measurement. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…measured The Arcade live suite ran against the live API for the first time: 9 passed, 3 failed, and not one failure was the vendor's. One was a product defect in the connection filter (fixed in 28b13e4); the other two, and a duration failure, were defects in this suite. All three are corrected from the measurement rather than from the assumption that preceded it. TIMEOUTS, because bun's per-test default is 5000ms. `next_uri is refused for every value` makes five sequential vendor round-trips and died at 5001.03ms having measured none of them. Every case that dials Arcade now carries an explicit timeout, following the pattern the consent case already used: 60s for a case, five times that for the five-call `next_uri` case, the poll's own bound for `auth.status`, and the same on the `afterAll` cleanup, which also dials. GATE 4 ASSERTED THE WRONG SPELLING, and that is how it hid the product defect. It filtered connections by the provider TYPE, found nothing, and concluded "the filter does not take this provider id". The filter is fine; `provider_id` means two different things: the TYPE on a tool listing (`google`), the INSTANCE on a connection row (`google-openbot`, `cpk-poc`, `arcade-google`), with the auth-provider config the only bridge between them. It now prints the consented connection's own `provider_id` verbatim — the line whose absence made the run ambiguous — resolves the instances through `admin.authProviders.list()`, asserts the connection is found by instance, and measures the case handling against the instance, which is the value a filter is actually given. GATE 2 MEASURED THAT ITS ASSUMPTION IS FALSE, so it now records that instead. Arcade answers a wrong-user `confirm_user` with a generic 400 `coordinator_error` / "Bad request", carrying nothing that identifies a mismatch, so the `{ mismatch: true }` contract is unreachable through the live API today; detecting it would mean reading every bad request as a mismatch. The product keeps that branch — it costs nothing and starts working if the vendor ever distinguishes the case — and the case asserts the measured reality: the vendor refuses, `confirmReturn` throws, and the return leg refuses with `vendor-unavailable`. A UX degradation, not a hole: the leg stays bound by the `auth_id` comparison regardless. Both other outcomes are still printed. Suite only; no product code is touched. The live suite was NOT re-run — it needs a person at a browser. Verified that it still type-checks against the server's config, still skips every case with no key, and that `test:live-arcade` still exits non-zero without one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`main` moved 14 commits past this branch's base and claimed `0053`/`0054` for its own migrations. Five files conflicted. The migration collision is the one that is not a text merge. This branch's `0053_adorable_frank_castle` and `0054_spicy_wallflower` were dropped and regenerated as a single `0057_arcade_provider` on top of main's lineage, because main's `0053`–`0056` are already applied wherever this deploys: replaying ours underneath them is not a thing a deployed database can do. The regenerated SQL is the exact union of the two it replaces — the `pending_authorizations` table with its cascade, and the `brokered_connections` provider check widened to include `arcade` — which is also the evidence that the two schemas collide nowhere else. `config.ts`: main hoisted `publicUrl` into a local const holding the identical expression; this branch added `arcadeApiKey` and `brokerProvider` beside it. Both kept. `connected-accounts/index.tsx`: two callbacks now report on this route. `?authorized=` is the brokered verifier's and `?linked=` is the GitHub account-link's, and they stay separate parameters for the reason the first one's comment already gives. `memory-defaults.integration.test.ts` is main's, and it built a plugin store without `brokerProvider`. That option is required and deliberately has no fallback — the store dispatches its toolkit read on it — so main's nine new tests died in `toolkitOfFor` the moment the two branches met. The fixtures are `composio://` rows, so they now say so. `server/tests` sits outside `tsc`, which is why a missing required option surfaced as a runtime failure and not a type error. Verified on the merged tree: `format:check` 0, `lint` 0, `typecheck` 0, `bun run test:server:sharded` 5028 pass / 0 fail / 19 skip, `bun test app/tests` 1246 pass / 0 fail. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`main`'s memory-defaults suite built a plugin store without `brokerProvider`. The option is required and deliberately has no fallback, because the store dispatches its toolkit read on it, so all nine of those tests died in `toolkitOfFor` the moment this branch and `main` met. The fixtures are `composio://` rows with `provenance: "composio"`, so they now say so. `server/tests` sits outside `tsc`, which is why a missing required option surfaced as a runtime `TypeError` rather than a type error. This belonged in the merge commit and missed it: the `git add` that should have staged it named a path that does not exist, which makes git stage nothing, and the error was suppressed. The merge commit's verification line describes a tree that had this fix applied but uncommitted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`bun test` runs every file in one process, a pool is never handed back, and nothing reaps an idle connection — so a file costs two connections from its first query until the run ends. PostgreSQL's 100, less the superuser reserve, is therefore reached by WRITING A FILE rather than by any file misbehaving, and these fifteen are the files that reached it. Measured, because the prose this branch shipped says otherwise. The sharded runner's docblock states the slots go "during the import phase"; constructing a pool opens nothing at all, the first query opens the pool's full `max`, and twenty pools sat flat at forty connections for as long as they were watched. Exhaustion is a function of how many files have RUN, which is why a sharded sweep never sees it and `test:ci` does. Half this suite already knew. Forty-three files on `main` close their pool in their own `afterAll`; fifty-five do not, and the fifteen added here were fifteen more. Closing them puts this branch back at `main`'s own number, which is the number CI passes at today. Nothing about the harness changes: no workflow edit, no shared pool, no new convention — these files now do what `identity-store.integration.test.ts` and forty-two others already do. The close goes last in each file's `afterAll`, after that file's cleanup, because hooks run first-registered-first and a pool closed before the deletes takes the deletes with it. Verified against a freshly migrated test database: `format:check` 0, `lint` 0, `typecheck` 0, `bun test app/tests` 1246 pass / 0 fail, `bun run test:server:sharded` 5027 pass / 1 fail / 19 skip, and `bun run test:ci` — the whole-repo single-process run CI performs — peaked at 84 connections against a ceiling of 97, with zero `too many clients`, where it had reported 114 such failures before. The one remaining failure is not this change: `arcade-journey`'s "two separately-shared apps" fails identically with and without it, and is the merge-gate item of that name. Connection exhaustion had been hiding it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`eventsOn` read the audit trail with no `ORDER BY` and the assertion below it
takes `.at(-1)`, meaning "the disconnect this test just caused". An unordered
`SELECT` does not answer that question. PostgreSQL may return such a read in
any order, and what it returns in practice is roughly heap order — which
agrees with time order right up until it doesn't.
It didn't on CI. The last row back was the PERSONAL disconnect from the
describe above, so the Shared app's entry was read as a person ending their
own account:
- owner: "deployment", reason: "admin"
+ owner: "arcade-journey-…", reason: "self", vendorRevocationRequested: true
THE PRODUCT WAS RIGHT THE WHOLE TIME. Both entries are written, correctly and
48ms apart — the personal one at `…42.954`, the deployment one at `…43.002`.
Only the reading of them was wrong, and it reported the product for it: an
audit misattribution on a Shared account, which is the kind of defect somebody
would rightly stop a release over.
Reproduced away from the suite, by making heap order disagree with time order
— insert the newer row first, then the older. Unordered, `.at(-1)` is the
older row; ordered by `createdAt`, it is the newer one. That is the CI failure
exactly.
`createdAt` is the only key that can carry this, because `id` is a random UUID
and sorts arbitrarily. The two disconnects arrive on separate requests and so
in separate transactions, which is what makes their `now()` values distinct.
This was invisible until today for a reason worth recording: the whole-suite
single-process run is where the two describes' rows meet, and that run has
been drowning in `too many clients` since before this branch existed.
Verified: `format:check` 0, `lint` 0, `typecheck` 0, `bun test app/tests` 1246
pass / 0 fail, `bun run test:server:sharded` **5028 pass / 0 fail / 19 skip**.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…from a zero
`@arcadeai/arcadejs`'s `OffsetPage` constructor does `this.total_count =
body.total_count || 0`, so an ABSENT `total_count` is indistinguishable from a
reported zero. Two defects fall out of that one coercion, both measured against
a stub HTTP engine with the real SDK building real pages:
- `connectionsFor` used the SDK's own auto-pagination
(`for await (… of client.admin.userConnections.list(…))`), and
`nextPageRequestOptions()` returns `null` the moment the total is falsy. With
250 connections served 100 to a page and no `total_count`, ONE request went
out: `revoke` deleted 100, ANSWERED `true`, and left 150 connections live at
the vendor — a person told the disconnect succeeded while it had not. That is
the outcome `revoke`'s own comment forbids. `isConnected` and `accountName`
read off page one the same way.
- `listApps` and the transport's `listTools` kept the LAST page's `total_count`,
which the coercion turned into `0` whenever the short final page omitted the
field. An engine truncating at 250 of 300 committed a 250-action listing as
complete, with no error.
Fixed at the root, not per call site:
- `reportedTotalOf` reads the total off the RAW parsed body (`AbstractPage.body`)
and falls back to the page property only when there is no body, so "the vendor
did not say" can never read as "the vendor said zero".
- `highestTotal` keeps the HIGHEST total any page reported, never the last, so a
short final page that drops the field cannot erase page one's claim.
- `pagedItems` reads one listing to its end: pages until a SHORT page, never
touches the SDK's auto-pagination, and the incompleteness throw is the last
thing it yields, so a caller folding items as they arrive still cannot see a
short listing finish as a whole one. A partial or unverifiable listing now
THROWS on every path — neither a clean success nor an empty "nothing there".
- `createArcadeClient`'s `actions.listTools` normalizes `total_count` at the
seam, OMITTING the field when the vendor omitted it. That is the only place it
can be done: `./arcade` must not import this file (cycle), so it cannot reach
`reportedTotalOf` itself.
Same-pattern audit over both files, every instance fixed:
- `listApps` (`arcade-adapter.ts`) — now through `pagedItems`.
- `authProviderInstances` — now through `pagedItems`.
- `connectionsFor` — was the SDK's auto-pagination; now explicitly paged.
- `grantsFor` — was a single `await` with no loop, and answers one `GrantState`
PER ACTION, so a first page silently dropped every action past it
(`GithubApi` publishes hundreds). Now paged.
- `arcade.ts listTools` — highest-total tracking.
- `authProviderOf` — LEFT UNPAGED, deliberately, and the docblock now records
why: it answers one fact for the WHOLE toolkit, so a further page can only
repeat the first, and paging it would be paid on every `isConnected`,
`accountName` and `revoke`.
Covering test: `server/tests/arcade-pagination-engine.test.ts` serves real JSON
over a real socket through `ARCADE_BASE_URL` and lets the SDK build its own
`PagePromise`/`OffsetPage`. The existing adapter suite stubs resource PROTOTYPES
and hands the adapter a bare async generator, which is exactly what hides this
class — a test in that style cannot go red here. Against the unfixed source this
file fails 5 of 7 (revoke spent 100 deletes of 250 and returned `true`); against
the fixed source it passes 7 of 7.
Symbols added, all module-private to `server/src/plugins/arcade-adapter.ts`,
exported by nothing, so they have no call sites outside it: `reportedTotalOf`,
`highestTotal`, type `Listing`, `pagedItems`, `CONNECTION_PAGE_SIZE`,
`CONNECTION_MAX_PAGES`. No symbol was renamed or removed and no exported
signature changed.
Call sites enumerated for what did change behaviour:
- `actions.listTools` return VALUE shape (a plain `{items, total_count?}` rather
than the SDK's `OffsetPage`; the declared `ArcadeActions["listTools"]` type is
unchanged). One consumer: `server/src/plugins/arcade.ts:179`
(`installed.listTools`), installed from `server/src/index.ts:797`. Test doubles
implementing `ArcadeActions` already answer plain objects and are unaffected.
- `broker.listApps()` — `server/src/plugins/logos.ts:20`,
`server/src/plugins/routes.ts:957`, `server/src/plugins/routes.ts:1066`.
- `broker.grantsFor(...)` — `server/src/plugins/routes.ts:2183`.
- `broker.isConnected(...)` — `server/src/plugins/store.ts:6887`.
- `broker.revoke(...)` — `server/src/plugins/store.ts:4076`,
`server/src/plugins/store.ts:7969`, `server/src/plugins/store.ts:8198`.
- `broker.accountName(...)` — `server/src/plugins/store.ts:5847`.
All of these could already receive a throw from this path — `authProviderOf` and
`authProviderInstances` throw on a malformed listing today, and `connectionsFor`'s
docblock already records that it propagates onto the Disconnect path — so no
caller needed new handling.
Three fixtures were taught the page shape the adapter now consumes, because they
stubbed `userConnections.list` with a bare async generator that has no `items`:
`arcade-adapter-connections.test.ts`, `arcade-adapter.test.ts`,
`arcade-journey.integration.test.ts`. The suite's structural blindness in general
is not addressed here; it is somebody else's finding.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsS7GKpBgupvAKampCVKAK
`PgDialect.migrate` decides what to run with one comparison — the journal's
`when` against the newest `created_at` in `drizzle.__drizzle_migrations` — and
says nothing about a migration stamped below that mark which it has never run.
Merging `main` into this branch renumbered the arcade work to `0057` and gave
main's `0053_identity_links` a `when` of 1791547580808, older than the
1791774497487 a pre-merge database had already recorded for the branch's own
`0054_spicy_wallflower`.
Reproduced against a throwaway database: the pre-merge folder applies, the
post-merge folder dies on `CREATE TABLE "pending_authorizations"`, and after the
obvious hand repair (drop that table, run again) the migrator prints
`{"type":"migrations-applied","status":"ok"}` while `identity_links` and
`identity_link_challenges` were never created. A fresh database is unaffected,
which is why CI — always migrating from empty — cannot see it.
The migrations themselves are untouched: `0057_arcade_provider.sql` is byte
identical to a fresh generate and the snapshot chain is intact, so renumbering
again would be a second defect. What changes is that the migrator now looks
before it runs. `migrationsOutOfReach` names every migration stamped at or below
the database's high-water mark whose hash is not among the rows already applied —
those are unreachable, not pending — and `applyMigrations` throws
`MigrationsOutOfReachError` with the names and two ways out instead of migrating.
Nothing is applied, so the hard failure and the silent skip that follows its hand
repair are both replaced by one refusal that exits non-zero.
It refuses rather than repairs on purpose: the migrations are out of reach
because some other journal already ran here, and nothing in this process knows
what that journal did to the schema. An operator recreating a disposable database
or restamping a durable one is a decision, not a side effect of a deploy.
Validated for false positives against two real databases: `openbot_test` (58 rows,
fully migrated with this folder) and the development `openbot` (53 rows, at 0052)
both report nothing out of reach, and every applied hash is accounted for by this
folder — confirming the sha256-of-the-whole-file recipe matches what drizzle has
been writing for the whole history.
Every migrator invocation now runs the guarded script, since the unguarded CLI
was reachable from the three environments that actually hold such a database:
`db:migrate`, the compose `migrate` service that `scripts/start.sh` runs, and CI.
`drizzle-kit generate` and `drizzle-kit check` are unchanged.
Audit: `server/drizzle` is the only migration folder and `server/drizzle.config.ts`
the only config in the repository, and after this change `server/scripts/migrate.ts`
is the only thing that applies them. `scripts/start.sh` keeps its two-table canary
after a successful migrate, which is the symptom-level version of this check and
now sits behind a cause-level one.
Call sites of every symbol added or changed:
- `readMigrationFolder`, `migrationsOutOfReach`, `outOfReachRefusal`,
`FolderMigration`, `AppliedMigration` (new, server/src/db/migration-reach.ts):
server/src/db/apply-migrations.ts, server/tests/migration-reach.test.ts.
- `applyMigrations`, `MigrationsOutOfReachError` (new,
server/src/db/apply-migrations.ts): server/scripts/migrate.ts,
server/tests/migration-reach.integration.test.ts.
- `migrate`/`drizzle` imports removed from server/scripts/migrate.ts: no other
module imported them from there.
- `db:migrate` keeps its name and its behaviour on a healthy database, so its
three mentions stand: README.md:430, docs/development.md:77,
scripts/start.sh:277.
- `bunx drizzle-kit migrate` had exactly two invocation sites, both replaced:
.github/workflows/ci.yml and docker-compose.yml.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsS7GKpBgupvAKampCVKAK
… fix cycle) Fixes 8-1 F3/F4: `OffsetPage` coerces an absent `total_count` to 0, which both ended pagination after page one and disabled the `seen < total` completeness check. `revoke` therefore returned true having deleted only the first page, telling a person the disconnect succeeded while connections stayed live at the vendor. Same family as the live defect fixed in 28b13e4. Orchestrator-verified, not taken on the agent's word: - red reproduced on 50aae80 + the new test alone: 5 fail / 2 pass, exit 1, including grantsFor `Expected: 250 Received: 100` - green on this branch: 7 pass, exit 0 - all 26 arcade test files on this branch: 216 pass / 0 fail, exit 0 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PsS7GKpBgupvAKampCVKAK
…et (a), round 1 fix cycle) Fixes 8-3 F1, a defect in merge commit 9c615eb. The renumber gave main's 0053_identity_links a `when` older than the stamp a pre-merge arcade database already holds, and drizzle's migrator gates purely on Number(lastDbMigration.created_at) < migration.folderMillis. Such a database first died on CREATE TABLE pending_authorizations, then after the obvious hand repair reported success while never creating identity_links or identity_link_challenges. CI could not see it: CI always migrates from empty. The guard refuses rather than repairs — the database ran a journal that no longer exists, so nothing in-process knows what it did to the schema, and applying SQL on top of that turns a recoverable database into an unrecoverable one. It fires before the transaction opens, so one refusal closes both halves. Orchestrator-verified end to end, not taken on the agent's word: - defect reproduced independently: unguarded migrator on a pre-merge database exits 1 on CREATE TABLE pending_authorizations, then after DROP TABLE reports {"type":"migrations-applied","status":"ok"} with identity_links and identity_link_challenges absent - guard on the same database: exit 1, names 0053_identity_links, journal count unchanged at 55, nothing applied - fresh database (the CI path): ok, exit 0, idempotent on re-run, all four tables present — no false positive - migrations byte-identical to base: git diff over server/drizzle/ is empty - 26 pass / 0 fail across the three migration test files Migrator invocation sites: all five now route through the guarded script (db:migrate, compose migrate, CI, image, Helm). server/Dockerfile WORKDIR is /app/server so `bun scripts/migrate.ts` resolves; postgres and drizzle-orm are runtime deps while drizzle-kit is dev-only, so this also removes a latent production fragility. drizzle-kit generate/check unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PsS7GKpBgupvAKampCVKAK
…e actor holds
`DELETE /servers/:id/authorize` resolved `:id` through `brokeredAccountFor` and
then threw the resolved id away: `authorizationAttemptFor` is keyed on the actor
alone, so a cancel issued from app B deleted an in-flight attempt on app A and
answered `{cancelled: true}` as though it had done what was asked. That is the
mint/claim/complete correlation this PR holds from the minting end, broken from
the cancelling end.
The route now compares the attempt's `server_id` against the raw path parameter
— the same value `POST /servers/:id/authorize` writes when it claims the row —
and answers `{cancelled: false}` when they differ, the same non-error as there
being no attempt at all. The deletion still matches on `(actor_id, created_at)`,
so the existing race guard is untouched.
No symbol was added, renamed, resignatured or removed: `authorizationAttemptFor`
and `discardAuthorizationAttempt` keep their signatures, so every call site of
both (routes.ts:2681 and :2828; store.ts's own definitions; the mint's
definite-failure discard at routes.ts:2628; the mocks in
server/tests/arcade-authorize-cancel.test.ts and server/tests/arcade-verify.test.ts;
server/tests/arcade-attempt-store.integration.test.ts) is unaffected.
Audited every other route in routes.ts: no other authorization-lifecycle route
resolves an identifier and then acts on an actor-keyed lookup. `/arcade/verify`
takes no path identifier and binds on `(actor, auth_id)` deliberately.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsS7GKpBgupvAKampCVKAK
…s decided
`activeProvider` tested the two API keys with `!== undefined`, so
`ARCADE_API_KEY=` with nothing after it, or a key that is all whitespace,
counted as a CONFIGURED provider. `selectProviderWiring` then built a vendor
client from `""` — the failure surfaced at the vendor, on somebody's first
connect, instead of at the setting that caused it.
Only `server/src/config.ts`'s `optional()` trim prevented this in the deployed
path, and nothing stated or tested that dependency. `activeProvider` takes
config-shaped VALUES rather than an environment, so every other caller (a test,
an embedder, a future loader) reached it untrimmed. What "configured" means for
a credential is this function's own question, so it is now answered there: a key
is present only if it is defined and not blank after trimming.
RED-GREEN: the new cases failed against the unfixed function —
`activeProvider({ arcadeApiKey: "" })` returned "arcade", and
`selectProviderWiring({ arcadeApiKey: "" }, …)` built an Arcade client
(factory called with a zero-length key) — and pass after.
Same-pattern audit of the whole of broker.ts: no other credential, key or
identifier is tested for presence in a way that admits a blank. `brokerProvider`
is a two-member union validated in config, not a free string; `brokerReturnUrl`
already refuses a whitespace-only address with `returnUrl.trim() === ""`;
`isFieldScheme` and `flagOf` test vendor-sent values against closed sets, not
presence. `provider-wiring.ts`'s `c.arcadeApiKey !== undefined` narrowing guards
are now unreachable with a blank key and are left as type narrowing.
No symbol added, renamed, resignatured or removed: `activeProvider` keeps its
name and signature, so all three call sites (server/src/index.ts:884,
server/src/plugins/provider-wiring.ts:44, server/tests/provider-selection.test.ts)
are unchanged. The startup refusal's sentence now reads "is not set to a key",
so that an operator who typed `ARCADE_API_KEY=` is not told the variable is
missing; docs/plugins/arcade.md quotes that sentence verbatim and was updated
with it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsS7GKpBgupvAKampCVKAK
`assertActivatable` tested `otherProviderRows <= 0` and returned. `NaN <= 0` is false, so a NaN did not return — it fell through to the refusal sentence and reached a person as the literal words "NaN brokered connections". A negative or fractional count did the same. Whatever produced such a value was not counting rows, and not knowing how many rows the other provider holds is not the same as knowing it holds none, so the guard now refuses on any count that is not a whole non-negative number of rows, and says the count could not be taken instead of repeating the sentinel back. The zero case is now `=== 0`, so the only path that returns is a real zero. The second half of the finding — that the call site's `ne(provider, wired)` skips NULL-provider rows — does not hold: `brokered_connections.provider` is NOT NULL in the schema and in migration 0052, and a NULL insert is refused by the database, so there is no row the inequality can hide. Rather than add an `IS NULL` arm that can never match, a test in arcade-provider-check pins the NOT NULL, so dropping it fails loudly next to the reason the count needs it. Call sites of `assertActivatable` (signature and name unchanged): the startup activation check in server/src/index.ts:812, and server/tests/provider-selection.test.ts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PsS7GKpBgupvAKampCVKAK
…decide `selectProviderWiring` returned null wiring — no broker, no transport seam, no word to anybody — for both keys set with `OPENBOT_BROKER_PROVIDER` unset. Nobody wants that state on purpose: the deployment comes up holding two vendor keys and brokering nothing, so every brokered feature is dead and the symptom surfaces much later and far away. It is now refused at boot, in the shape of the refusal already beside it, naming the two keys and the variable and never a value. Same-pattern audit of the whole file: the two vendor branches each carried `&& c.<vendor>ApiKey !== undefined`, a narrowing that doubled as a silent exit to the same null wiring. The narrowing is kept as `requiredKey`, which throws instead of falling through; it cannot fire today, because `activeProvider` selects only from a key it has seen, but if the two ever disagree the deployment says so rather than unwiring itself. The one remaining null wiring is returned from the branch that means it — no keys at all — and is no longer reachable by falling through. Symbols: `requiredKey` added, module-private, called twice, both in this file. `selectProviderWiring` keeps its name, signature and return type; its three call sites (`server/src/index.ts:792`, and `server/tests/provider-selection.test.ts`) are unchanged in shape. `activeProvider` is untouched: it is a query, and the refusal belongs to the wiring. `docs/plugins/arcade.md` said "with both set and the variable unset, neither is active" and listed two refusals; it now states the third and quotes its sentence. Not changed: `provider-wiring.ts:61` building a client from an empty-string key, which is a separate finding owned elsewhere. The `!== undefined` semantics are preserved exactly here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PsS7GKpBgupvAKampCVKAK
…ening it
The D31 gate exists to prove that a bearer URL and an access token reach no
model, log line, error body, audit row or redirect. It read each captured
console argument through `String(arg)`, which renders an object as
"[object Object]" — so a token logged INSIDE an object passed it, and the
status reply the case feeds through carries one in `context: { token }`. The
gate could not see the very shape it constructs.
Every value is now WALKED by `harvest`: array and set members, map keys and
values, own property names and their values, an error's message, name, stack
and cause, and the rendering of anything with a `toString` of its own. The
audit and connection rows go through it too, since `JSON.stringify` is blind
to a Map, a Set and an Error's message. The answer is checked WHOLE — every
header and the body — rather than `Location` alone, in the D31 case and in the
vendor-unavailable case whose error message carries the token.
Watched against a planted leak: `console.log("...", { context: answer })` in
`settledStatus` passed the old gate and fails the new one, naming the token.
No real leak: the route is clean on all four surfaces.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsS7GKpBgupvAKampCVKAK
…oth directions of the cross-provider guard
The two store provider fixtures modelled a deployment that cannot exist. Both
wrote `vendor: "arcade"` / `vendor: "composio"` — the lower-case provider key,
which is what the PROVENANCE column holds. Production mints the vendor column
from `BROKERED_PROFILE[provider].vendor`, which is `"Arcade"` / `"Composio"`.
Both files now take that word from `BROKERED_PROFILE` rather than spelling a
second, wrong one; nothing on these paths reads the column, so this buys
fidelity rather than coverage, and it is recorded as such in the report.
The guards themselves were each asserted in one direction only:
* `store-toolkit-dispatch.test.ts` proved an `arcade://` row answers nothing
under a composio store, and nothing at all about a `composio://` row under
an arcade store. The dispatch is a per-provider table, so one entry proved
is one entry proved.
* `store-provider-required.test.ts` proved a composio store does not read an
arcade connection filed under the same app name, and not the mirror.
A test for each missing direction is added.
No symbol added, renamed, resignatured or removed: both files gain a value
import of `BROKERED_PROFILE` from `../src/plugins/access` (already exported and
already imported by `store-scheme-sites.test.ts`), and two `test()` cases. No
call site anywhere changes.
Break-the-guard evidence (source reverted after each):
* `toolkitOfFor` made to try both parsers — the new dispatch mirror fails
with `Received: { toolkit: "gmail", accounts: [] }` where null is required.
* the `provider` predicate dropped from `accountsOn`'s connection read — the
new provider-required mirror fails with the foreign `other-*` account
appearing in the slice.
The `@ts-expect-error` pin at `store-provider-required.test.ts:59` was
re-verified after the edits: with `brokerProvider` made optional in
`PluginStoreOptions`, `tsc` reports `tests/store-provider-required.test.ts(59,1):
error TS2578: Unused '@ts-expect-error' directive.` — it still errors for the
intended reason, and the probe was reverted.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsS7GKpBgupvAKampCVKAK
…crosecond
`pending_authorizations.created_at` is a `timestamptz`, which PostgreSQL keeps to the
microsecond; the `Date` the driver hands a caller back carries milliseconds. A row stored
at `…13.407577` reads back as `…13.407Z`, and `recordAuthorizationMint` and
`discardAuthorizationAttempt` matched it with an exact `=` against that `Date`, which
matched zero rows. `claimAuthorizationAttempt` works around this by writing
`date_trunc('milliseconds', now())`, but the COLUMN DEFAULT is `defaultNow()` at full
precision — what drizzle's inferred insert type invites any other writer to take — so a
row written that way could never be matched, no `auth_id` was ever written, and per the
table's own docblock "an attempt holding null can never complete": a silently dead
authorization.
The same two call sites also raised `TypeError: value.toISOString is not a function` from
inside `PgTimestamp.mapToDriverValue` when handed a `createdAt` that had been through
JSON — typed `Date`, carrying a string.
Both now go through `attemptNamed`, which truncates the COLUMN in the comparison as well
as the value, so a row is reachable however it was written, and through `attemptInstant`,
which accepts an ISO string and refuses anything naming no instant with a message that
says which field was wrong instead of a driver `TypeError`.
Fixed in the store rather than in the schema deliberately: `.default(sql\`date_trunc(...)\`)`
would change generated DDL and need a migration on a branch whose migration state is
delicate, and it would still only hold for rows written AFTER it — the matcher holds for
every row, whatever wrote it. Truncating cannot match the wrong row: `actor_id` is the
table's primary key, so there is at most one attempt per actor and `created_at` is the
staleness guard on that one row, not a selector among several; a value naming an older
attempt is still refused, which is what stops a late discard deleting the replacement.
Signature change: `recordAuthorizationMint` and `discardAuthorizationAttempt` widen
`createdAt` from `Date` to `Date | string`. Both are called only from
`server/src/plugins/routes.ts` (lines 2628, 2647, 2683), all three passing a `Date`, and
from `server/tests/arcade-attempt-store.integration.test.ts` and
`server/tests/arcade-completion-transaction.integration.test.ts`. A widened input breaks
no caller. New non-exported helpers `attemptInstant` and `attemptNamed` have no other call
sites.
Audit: the only timestamp column in `server/src/db/schema/plugins.ts` used as an equality
key anywhere is `pendingAuthorizations.createdAt`; every other one (`updatedAt`,
`connectedAt`, `verifiedAt`, `toolsRefreshedAt`, `publishedAt`, `grantedAt`,
`completedAt`) is written, ordered by or range-compared, never matched exactly. No DDL
change, so no migration.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsS7GKpBgupvAKampCVKAK
`textOf` claimed to answer "is this text" and answered yes for `""`. Every
caller that chains it with `??` then ends on the empty field, because `??`
falls through on null and undefined only: a connection row carrying
`provider_user_info: { name: "", login: "maxim" }` yielded NO account name at
all, and the Connected-accounts screen drew a blank where the identity goes on
a connection that is genuinely connected.
The emptiness belongs in `textOf`, not in a `||` at each call site — one of
which would be forgotten. Whitespace-only is the same blank to whoever reads
the screen, so it answers the same. The value itself is returned untouched:
this says whether there is text, it does not reformat the vendor's.
No symbol was added, renamed, resignatured or removed. `textOf` is module-local
(never exported) and has ten call sites, all in this file; behaviour changes at
three of them and is unchanged at the other seven:
- CHANGED L692 `accountName`: `textOf(info?.name) ?? textOf(info?.login)` now
reaches the login. The defect.
- CHANGED L787 `authorizations.status`: an empty `status` reads `"unknown"`
rather than `""`. routes.ts compares it to "completed"/"failed"/"pending",
so the refusal it produces is the same; the value is now honest.
- CHANGED L788 `authorizations.status`: an empty `user_id` reads `null` rather
than `""`. The route's `settled.userId !== attempt.vendorUserId` mismatch
verdict is unchanged either way.
- unchanged L49/L50 `normalizeArcadeError`: `name`/`message` become undefined
instead of `""`. `arcade.ts`'s `arcadeFailure` then answers null for such a
thrown value, and `callTool` returns the same "could not run" sentence,
because `"" !== "tool_authorization_required"` on both sides.
- unchanged L329/L330 `authProviderInstances`: already guarded by `instance &&`,
and `wanted` can never be empty (`authProviderOf` only adds truthy providers).
- unchanged L417 `connectionsFor`: `instances` holds no empty id, so an empty
`provider_id` matched nothing before and is skipped now.
- unchanged L460 `grantStateOf` action, L469 secret key: both already `!x`.
- unchanged L504/L505 token status/status: compared to literals, and `""` is
falsy exactly where `undefined` was.
Same-pattern audit of the whole file: `listApps` had the one other presence
test that admitted an empty value — `typeof toolkit.name !== "string"` let a
toolkit named `""` raise a blank, unconnectable row whose slug is what a URL is
built from. It now skips, through the same `textOf`. The description beside it
keeps its blank default, which is a label and not an identity. `authorize`'s
`url` and id checks already spelled this rule inline and are left alone; scope
arrays are vendor data echoed verbatim and are not a presence test.
Red-green: with the fix reverted, the three new assertions fail —
accountName answered `null` for `{ name: "", login: "maxim" }`, status answered
`""`, and listApps answered `["Github", "", " "]`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsS7GKpBgupvAKampCVKAK
… page the SDK built
`OffsetPage`'s constructor does `this.items = body.items || []`, so every reader of the SDK's PAGE
OBJECT sees an array whatever the vendor sent. Three guards read items off the page and so could
never fire for the case they were written for:
- `pagedItems` — its `!Array.isArray(items)` refusal was unreachable, which made the `malformed:`
sentence of all four of its callers (`listApps`, `grantsFor`, `authProviderInstances`,
`connectionsFor`) dead text; a listing answering `{"error":"nope"}` resolved as an empty one.
- `authProviderOf` — the one unpaged listing, same read, same dead sentence.
- `createArcadeClient.listTools` — handed the transport the manufactured `[]`, so the transport's
own guard (`arcade.ts`) was dead too and a refresh against a malformed listing would have cleared
an app's recorded actions to none.
C3 already taught `reportedTotalOf` to read `total_count` off the raw parsed body for the same
reason (`body.total_count || 0` hides an absent total). That read is extracted here as `sentBody`
and reused for items as `sentItemsOf`, keeping C3's fallback: an answer with no `body` is read
directly, which is what `admin.authProviders.list` (a plain `APIPromise`, not a page) and every
hand-built stub return.
Audit of the same pattern: `OffsetPage` manufactures exactly `items`, `total_count` and `offset`;
no other class in the SDK manufactures defaults. `total_count` was already handled; `offset` is
never read off an answer in either file (both track their own). Everything else is read off row
objects, which are plain parsed JSON.
RED-GREEN: `server/tests/arcade-malformed-listing-engine.test.ts` serves `{"error":"nope"}` over a
real socket through `ARCADE_BASE_URL` and lets the SDK build its own pages — 6 of its 7 tests
failed before this change ("Received promise that resolved"), all 7 pass after.
`arcade-pagination-engine.test.ts` stays green. The existing prototype-stubbing adapter tests are
structurally blind to this class, which is why a test in that style is not used.
Call sites (nothing renamed, removed or resignatured):
- `sentBody` (new, private): called by `reportedTotalOf`, `sentItemsOf`.
- `sentItemsOf` (new, private): called by `pagedItems`, `authProviderOf`, `listTools`.
- `reportedTotalOf`: signature and behaviour unchanged; callers `pagedItems`, `listTools`.
- `createArcadeClient`'s `listTools` return type unchanged, so `ArcadeActions` is untouched.
Not changed: a JSON body of literal `null` still escapes as the vendor's own TypeError from inside
`OffsetPage`'s constructor; catching it would mean wrapping the page call and swallowing genuine
`APIError`s the authorize path depends on.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsS7GKpBgupvAKampCVKAK
Orchestrator-verified red-green in a throwaway worktree at the base commit: red non-zero on unfixed source with the fix's own test, green 0 on the branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PsS7GKpBgupvAKampCVKAK
Orchestrator-verified red-green in a throwaway worktree at the base commit: red non-zero on unfixed source with the fix's own test, green 0 on the branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PsS7GKpBgupvAKampCVKAK
… it connected A user-connection row carries `connection_status` — the field is in the key set this file quotes verbatim at `connectionsFor`, and the SDK declares it (`connection_status?: string`) — and NOTHING read it. The adapter took a row's mere EXISTENCE as the answer, so `isConnected` answered `true` and `accountName` answered a name for a connection Arcade was reporting as expired, revoked at the provider, pending or failed. Nothing presented as wrong until a tool call failed later, far from the screen that said Connected, with no way back but guessing. The seam's own word for `isConnected` is "currently attached to this app at the vendor", and a finished connection is not that. THE DESIGN, AND WHICH WAY EACH UNKNOWN FALLS: - A row is LIVE when its `connection_status` is `"active"` — the one value measured on a usable connection at the live API, and the spelling this tree's own engine fixture already carried. An ALLOWLIST of the live spelling and not a denylist of dead ones: the SDK types the field as a bare string with no union, so the dead vocabulary cannot be enumerated from here, and a denylist missing one spelling would go on reading that spelling as connected — the defect itself. - A row with NO status, or an empty one, still reads as LIVE. The field is optional on the wire, so a pessimistic reading would answer "not connected" for every real connection against any engine that omits it: `isConnected` false, `accountName` null, `revoke` false WITHOUT SPENDING A DELETE. That is exactly the live defect 28b13e4 fixed, re-entered one field along, and it is the worse direction — a live connection reading as absent strands somebody with no path back, where a live-but-unrecognised status at worst sends them round a consent they already gave. `grantStateOf` already draws this line: not knowing is not the same as not connected. - `revoke` ASKS ON EVERY ROW, live or dead, which is why the liveness test is not a filter inside `connectionsFor`. A dead row is still a row at the vendor; skipping it would leave it standing while `revoke` answered `false` ("there was nothing to withdraw"), and a later reconnect would be made beside debris nobody can see. The two kinds of caller need opposite things from the same row, so the listing stays whole and only the POSITIVE reads ask the question. - Folded and trimmed, Amendment 16's client-side fold, as every other comparison in this file is. THE GRANTS VIEW NEEDS NOTHING HERE: `grantsFor` answers out of the per-person tool listing's own `requirements.authorization` (`token_status`, `status`) rather than out of connection rows, and `grantStateOf` already reads both negatives. SAME-PATTERN AUDIT OF THE WHOLE FILE — one other vendor field is named in a docblock and never read, and it is DELIBERATELY left unread: an auth-provider config's `status` (`"status":"active"` in the verbatim block at `authProviderInstances`). That listing is the ATTRIBUTION BRIDGE — it says which vendor a connection row belongs to — not a liveness claim about anybody's connection. Dropping inactive instances from it would make rows under them UNATTRIBUTABLE, which does not merely read as "not connected": it also stops `revoke` from deleting them, so an operator disabling a provider configuration would silently make everybody's Disconnect a no-op. Whether a person's connection still works is the connection row's own `connection_status`, and that is now read. Everything else the file touches already reads its negative: `grantStateOf`'s secret `met`, `token_status`/`status`, the authorize answer's `url`/`id`, and `requirements.met`, whose non-reading is argued in place. RED-GREEN, against `arcade-pagination-engine.test.ts`'s real-socket engine rather than the prototype stubs, which cannot see this class at all. Red, verbatim: (fail) ... isConnected and accountName refuse a row whose connection_status is not live Expected: false / Received: true (fail) ... a live row beside a dead one answers for the live one Expected: "person 1" / Received: "person 0" Four cases now stand: a dead row is not a connection; a live row beside a dead one is the one answered for; a row with no status at all still connects; and `revoke` deletes both and says it asked. CALL SITES of everything added or resignatured, all of them module-private: - `connectionIsLive` (new): `broker.isConnected`, `broker.accountName`. Nothing else, deliberately — `revoke` above. - `LIVE_CONNECTION_STATUS` (new): `connectionIsLive` only. - `connectionsFor`: unchanged signature and unchanged answer. - test helper `connectionRow` gained an optional second argument (one call site, in its own file), `EngineOptions` an optional `connectionStatuses`, and `statusOf` is new beside them. No exported symbol changed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PsS7GKpBgupvAKampCVKAK
Orchestrator-verified red-green in a throwaway worktree at the base commit: red non-zero on unfixed source with the fix's own test, green 0 on the branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PsS7GKpBgupvAKampCVKAK
`pending_authorizations` declared two TypeScript unions — `holder` as "person" | "deployment" and `outcome` as the four-name roster — and the migration that created it, `0057_arcade_provider.sql`, added no CHECK for either, in the same file that drops and re-adds its sibling's `brokered_connections_provider_check`. `$type<>` is erased at runtime, so the database accepted 'llama' as a holder and 'banana' as an outcome and drizzle handed both back typed as the declared unions: a `switch` over `outcome` the compiler believes exhaustive was not. The same table also permitted exactly the two holder/`user_id` combinations its sibling forbids — a person attempt with a null `user_id` and a deployment attempt carrying one — because `user_id` is nullable, so `InferInsertModel` makes it optional and TypeScript raises nothing. `brokered_connections_holder_check` exists precisely to make those impossible; the new table now carries the same pair, under the same names. Two CHECKs, modelled on the sibling and on `mcp_servers_account_mode_check` for the nullable column: pending_authorizations_holder_check pending_authorizations_outcome_check Migration `0058_pending_authorization_checks.sql`, generated with `drizzle-kit generate`. `0057`, `_journal.json`'s existing entries and every existing snapshot are untouched; the journal gains only its append, and `0058_snapshot.prevId == 0057_snapshot.id`. `drizzle-kit check` passes. Applied cleanly to a fresh database and to the already-migrated `openbot_test`. No row can violate either CHECK: the table is created by 0057 on this unmerged branch, both local databases hold zero rows, and its only writer, `claimAuthorizationAttempt`, already writes `user_id` only for a `person` holder and `outcome` only from the roster — the CHECKs are the backstop for a hand-written statement or a cast, not a correction. No symbol was added, renamed, resignatured or removed. `pendingAuthorizations` keeps its name, its columns and its inferred types; the `pgTable` call simply gains the extra-config argument the sibling already passes, so no call site changes. `bun run typecheck` is green. Whole-of-plugins.ts audit for the same pattern: the other three `$type<>` unions are already held — `mcp_servers.account_mode` by `mcp_servers_account_mode_check`, `brokered_connections.provider` by `brokered_connections_provider_check`, and `brokered_connections.holder` by `brokered_connections_holder_check`, whose two branches pin it to the pair. These two were the only unheld ones. Deliberately left alone: `mcp_servers.provenance`, which is plain `text` with no declared union and so makes the compiler no promise, and the many `$type<>` unions in the other schema files, which are a different finding and a different table. Tests: server/tests/arcade-attempt-constraints.test.ts, run red against a database holding 0057's shape (every refusal read "accepted") and green after 0058. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PsS7GKpBgupvAKampCVKAK
Conflicted with A2 in provider-wiring.ts and docs/plugins/arcade.md; both had rewritten the same refusal sentences. Resolved to A3's structure with A2's semantics, which is not a mechanical pick of either side: A3's new both-keys refusal tested `c.composioApiKey !== undefined && c.arcadeApiKey !== undefined`. A2 had just established in the same cycle that `!== undefined` is the wrong notion of a configured credential, because a blank key passes it. Taken together unchanged, a deployment holding TWO BLANK keys would have been told "COMPOSIO_API_KEY and ARCADE_API_KEY are both set" — a false sentence, and precisely the class of defect A2 was fixing. So A2's predicate is hoisted out of activeProvider and exported from broker.ts as `credential`, and provider-wiring's both-keys test now uses it. One definition, because the two call sites must agree exactly; the P0 DRY sweep had already flagged same-named helpers drifting apart in this diff. Orchestrator-verified after resolution: - typecheck exit 0 - provider-selection + arcade-provider-check: 24 pass / 0 fail, exit 0 - the resolved edge, checked directly: two blank keys with nothing named wire to nothing (the "no keys at all" branch), rather than raising the false both-are-set refusal Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PsS7GKpBgupvAKampCVKAK
…only to its operator `brokerReturnUrl` spelled `Composio` as a literal in both refusals and took no provider, so it could say nothing else. The deployments that can actually reach the authorize route's return-address check are Arcade ones — `routes.ts` gates that route on `broker.authorizations`, which only Arcade declares — so the one audience for this sentence was told its failure involved a vendor it does not use. The same sentence handed that reader `OPENBOT_APP_URL`, an environment variable only an operator can set, on two `requireUser` routes. The refusal is now written twice, the division `BrokerUnconfiguredError` already draws between a `person` and an `operator`: `message` — what the route answers with — names no vendor and no variable and says an administrator can correct the address, and `operatorMessage` names the ACTIVE provider (from `BROKERED_PROFILE`) and the variable, and is logged rather than answered. Had the operator's half simply been dropped, a misconfigured address would have become invisible to the only reader who can fix it. CALL SITES OF `brokerReturnUrl`, ENUMERATED. Two in `src`, both in `server/src/plugins/routes.ts`, both `requireUser` and both now passing `activeProvider` and logging the operator's half through `logReturnUrlRefusal`: - `POST /servers/:id/connect` (the brokered branch, mint of a connect link) - `POST /servers/:id/authorize` (the per-action authorization mint) Seven in `server/tests/composio-broker-seam.test.ts`, updated. No other caller exists in `server/`, `app/`, `worker/` or any script; `BrokerReturnUrlError` was constructed nowhere but inside `brokerReturnUrl` itself. SAME-PATTERN AUDIT OF `broker.ts`. The only other message-bearing strings in the file are `BrokerUnconfiguredError` (already provider-keyed and reader-split) and `assertActivatable`, which names the vendor it is actually refusing, keyed on its `candidate` argument, in a boot-time refusal no person can reach. Neither is this defect. Comments that said "Composio" where they meant "the vendor" were corrected where they sat on this code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PsS7GKpBgupvAKampCVKAK
… in it A crafted `?authorized=` on this deployment's own origin broke the Connected accounts screen. `AUTHORIZATION_NOTICES` was an object literal, so the lookup walked `Object.prototype`: `?authorized=toString` answered with a function, `constructor` with another, `__proto__` with an object. None of them is nullish, so the `?? null` that exists to mean "a code this build does not know draws nothing" never caught them, and `Record<string, string>` told the compiler the case could not arise. The page then handed React whatever came back — an empty red alert for a function, and for `__proto__` "Objects are not valid as a React child", which took the whole screen down for whoever opened the link. Filtering three names would leave the declared type a lie. These maps are now `ReadonlyMap`s instead: `.get()` answers with what was put in the map and nothing inherited, and it returns `string | undefined`, so the `?? null` and `authorizationNoticeFor`'s `string | null` are true statements rather than claims the compiler takes on trust. The same shape, keyed by a string from outside this deployment, is fixed wherever it appears: - `AUTHORIZATION_NOTICES` and `MARKS` (connected-accounts/index.tsx), keyed by a search parameter and by a catalogue key. - `FUNCTION_FOR` (gallery/activity.tsx) and its server mirror `ACTIVITY_FUNCTION_FOR` (components/headless.ts), keyed by a model's `report` argument; a borrowed member passed the `functionName ?` guard and was handed to the grant check as the data function the component reads. - `EXPORTABLE` (plugins/google-drive-rest.ts), keyed by the mime type of a file somebody else uploaded and named; a borrowed member made an unreadable file look like an exportable editor format, skipping the refusal and sending a function to Google as the export format. - `FIXES` (intelligence-entitlement-check.ts), keyed by Intelligence's own error code; a borrowed member won the `||` and left `fix` a function where the advice sentence belongs. Red first in every case, against unfixed code, then green. Symbols: no symbol is added, removed or renamed. Four module-private constants and two exported ones change type from an object literal to `ReadonlyMap`. Call sites, all updated: `AUTHORIZATION_NOTICES` — `authorizationNoticeFor` in the same file and `app/tests/connected-accounts-authorized-notice.test.tsx` (the only importer; `Object.keys`/`Object.values` there become `.keys()`/ `.values()`); `MARKS` — `markFor`, same file, no other reader; `FUNCTION_FOR` — `ActivityReportCard` and the gallery entry's `reads`, same file; `ACTIVITY_FUNCTION_FOR` — one `reads`, same file; `EXPORTABLE` — one read in `read_file_content`; `FIXES` — one read in `entitlementWarning`. Deliberately left alone: `AUTH_ROUTE_EVENTS` in server/src/app.ts, which is the identical pattern on a caller-controlled request path and is being handled outside this change; and the maps keyed by this deployment's own closed sets (`DECISIONS`, `DISCOVERY_REASONS`, `PROVIDER_NAMES`, `RUN_LABEL`, `TRANSPORT_LABEL`, `ORDER`, `browser-activity`'s `labels`, already guarded by `Object.hasOwn`). The settings page's `?connected=`/`?authorized=` surviving a reload, noted by a reviewer on index.tsx:209, is untouched. The one import reordered in connected-accounts/index.tsx is the formatter's doing: the file did not pass `biome check` before it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PsS7GKpBgupvAKampCVKAK
The baseline was keyed `<file>:<line>`, so adding a line above any baselined error in a test file turned every error below it into a "NEW" one and failed CI. 9db29b1 was exactly that: 22 reported errors, all the same errors at shifted lines. A gate that cries wolf on every edit teaches people to run `--update` without reading, which is how real debt gets silenced. Entries are now `<file>: error TS<code>: <message>` with no position, the same discipline `scripts/mutation-register.ts` uses. The comparison was already a multiset, so nothing is lost: two identical errors in one file need two entries, and a third fails. On failure the report still prints the line(s) each new key occurs on. The regenerated baseline is the old one with line numbers stripped, byte for byte after sorting: 267 entries, none added, none removed. A missing baseline now fails with that diagnosis instead of reporting all 267 frozen errors as new. Verified: `bun run typecheck:tests` passes; with two blank lines prepended to agent-endpoint.test.ts (12 baselined errors shifted) it still passes; with a fresh TS2322 plus a third copy of a twice-baselined TS7006 added, it fails and names both. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…data step 0057 to 0063 were unreleased, and 0058 to 0062 only reshaped the table 0057 created. They are now: - 0057_arcade_provider: generated, the whole schema change. - 0058_arcade_provider_backfill: --custom, the two data rules no generator can write (brokered account_mode, mcp_tools.vendor_name). 0060's hand-written orphan DELETE is gone: it only cleaned pending_authorizations rows, a table nothing released ever had, and the keys it made room for are now created with the table. mcp_tools.vendor_name ships nullable (fix for a rolling deploy). The old 0063 added it NOT NULL and dropped its default in the same release, so an old replica refreshing a listing after the migration would fail its insert. Per docs/development.md the column stays nullable, the backfill fills existing rows, every reader resolves it through vendorNameOf (coalesce(vendor_name, name)), and tightening it is a later release. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…using the listing
`storableTools` threw on any name outside `^[a-zA-Z0-9_-]+$`, for every
transport. The MCP spec allows a dot in a tool name, so a custom MCP server
with one dotted action stored none of its others on an add, and an existing
server's list froze on every refresh after (`refreshTools` maps the throw to
"nothing was refreshed").
`storableTools` now returns `{ rows, skipped }`: an unofferable name is left
out and reported once, as the vendor spelled it, and every other action is
stored. `refreshTools` stamps the refresh, writes the skipped names into
`lastError` (the sentence the Plugins page and the refresh route already
show, capped at 400) and logs a warning with the server id and every name.
A listing where nothing survives the guard is treated like the empty
listing: actions already held are kept rather than deleted, as the old throw
kept them. Arcade's adapter still relies on the guard as its safety net for
names it does not recognise; those now cost one action instead of the whole
toolkit, and still never reach a model.
The typecheck baseline is re-keyed for lines the new integration tests moved;
the (file, code, message) multiset is unchanged, 267 entries before and after.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…s shared grant The /arcade/verify freshness check read any refusal from brokeredAccountFor as "the account moved" and withdrew the attempt's account. For a Shared app that account is the deployment's live one, so an administrator who lost the role mid-consent ended the team's grant at the vendor and cleared every sibling app's row. A refusal is now told apart from a move by re-resolving with readOnly, which lifts only the admin requirement. Only a different account withdraws; a refusal over the same account (or none) closes the attempt unreconciled, withdraws nothing, logs arcade-completion-not-permitted and refuses with the new not-permitted code. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…turn leg
completeAuthorizationAttempt returned nothing when its conditional UPDATE
claimed zero rows, so /arcade/verify redirected to the success landing even
when the person's Cancel had deleted the attempt between confirmReturn and
the UPDATE: told connected, audited cancelled, grant live at Arcade.
The store now answers { claimed }. On false the route asks
authorizationAttemptCompleted: a rival completion still lands, anything else
refuses with the new cancelled code and withdraws the grant at the vendor,
with siblings resolved under the same bound as the freshness branch. The
revoke is skipped (and arcade-cancelled-grant-left-live logged) where it
would end more than the cancelled consent: a connection that predates it,
or siblings on the same connection. The attempt row is left as the
canceller settled it.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…lly prints it docs/deployment.md and docs/development.md quoted "the migrator only runs migrations stamped after the newest one it finds"; the refusal built in server/src/db/migration-reach.ts says "... stamped after the row it reads". An operator searching the docs for the sentence on their screen found nothing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Nothing checked that the Arcade project's custom user verifier points at this deployment. Left at Arcade's built-in verifier, the deployment's verify route is never called: no consent is bound to the attempt that started it, checked against the identity it was minted for, recorded or audited by that leg, and nothing in this deployment's logs says why. It cannot be checked. The pinned SDK (@arcadeai/arcadejs 2.4.1) exposes only auth providers, secrets and user connections under admin, and Arcade's documented API has no read of the project's verifier setting (only a validate_custom_verifier that starts a test flow). So every boot with Arcade active now logs a warning naming the exact address to set — OPENBOT_PUBLIC_URL (or BETTER_AUTH_URL) + /api/plugins/arcade/verify — and pointing at docs/plugins/arcade.md; with no public origin it says which variables are missing instead of printing a placeholder URL as if real. The docs' verifier section now says plainly what the default does: the verify leg never runs, and what was observed by hand is that Arcade's built-in verifier refuses ids that are not project members, which OpenBot's never are. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…unbuilt sunset flow The activation refusal (broker.ts assertActivatable), the both-vendors refusal (provider-wiring.ts storedProvider) and docs/plugins/arcade.md all told operators to end the other provider's connections "through its sunset flow". No such flow exists. What does exist, and now is what the activation refusal names: keep the old provider active (its key set, OPENBOT_BROKER_PROVIDER naming it — legal with both keys set, and the startup count then sees zero foreign rows) and end its connections through the ordinary paths, each of which revokes at the vendor and deletes the row: a person's Disconnect, an administrator's Disconnect of a Shared app's account, removing an app (every account connected to it), or offboarding. There is no single action that ends them all, and it says so. Rows at BOTH vendors has no way out from inside the deployment: activating either provider is refused while the other holds rows, and nothing active is refused too. That refusal now says so and names the manual exit (end one vendor's accounts in its own dashboard, delete its brokered_connections rows, name the other) instead of pointing at a flow. The two "whoever builds the sunset flow" comments in plugin-store/connections.ts say no such flow exists. Also: "Three refusals" in the docs was followed by four bullets; it says four. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…I takes none The page said results were "measured… against the live API" and that everything marked measured "was observed against a live Arcade project", without saying that nothing in CI dials Arcade. What the repository actually records: - the toolkit counts, GitHub's 43 of 43, Google's first 11 of 30 and the default-verifier failure were found by hand before the page was written on 2026-10-12, with a throwaway script not in the repository (the page itself called it "the throwaway harness"); - server/tests/arcade-live.test.ts was run once, by hand, on 2026-10-13 (de98696, ffef56d): 9 passed / 3 failed, all three the suite's own mistakes, corrected afterwards and not re-run. It produced the provider_id shapes, the shared Google connection, Google's 0 -> 11 of 30 and the wrong-user 400. It skips every case without ARCADE_API_KEY, and no workflow sets one. Each measured claim now says which of those it came from; none was removed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
assertion-shape: the authorized-notice allow-list entry moved from line 101 to 103 when two refusal codes (not-permitted, cancelled) joined the list above it. Same offence, new line. fixture-identity-distinct: arcade-verify.test.ts gains two ACTOR|ACTOR collapses, 3 -> 5. Both are the new item-1 and item-3 cases. They seed or complete a person's account, mirroring accountRefFor's person ref, where the vendor identity is the local id. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@davidmckayv thanks. Every item is addressed and pushed as of Must fix
Also
|
davidmckayv
left a comment
There was a problem hiding this comment.
Thanks, this is a solid round. I checked each fix against the code rather than the reply. For each one I reverted the fix in a scratch copy and confirmed the new tests fail. I also ran the migrations on a throwaway database: an upgrade from 0056, a fresh install, an old replica inserting mid-rollout, and 500k rows. All four must-fix items hold, and so does the "also" list. The ratchet moves are within their own rules.
A few things are left before I approve.
Must fix
-
A mode flip mid-consent deletes the team's live shared row.
accountRowinserver/src/plugins/plugin-store/context.ts:345matches a deployment row on provider, app and holder, but notvendor_user_id. So when the withdraw branch clears the retired shared identity (route-groups/connections.ts:2349), it deletes whichever deployment row the app has. Here's how to reproduce it:- An admin starts consent on Shared identity A.
- The app goes Shared, then Personal, then Shared again, which mints identity B.
- Someone connects B, and then the admin returns.
The vendor revoke correctly targets A, but B's row is deleted. The app then reads as disconnected while B is still live at Arcade. A mode switch doesn't close open attempts, so this is reachable. Add
vendorUserIdto the deployment half ofaccountRow. This was already there before the last round; fix 1 didn't cause it. -
Pin each skip-revoke guard on its own. Removing just the sibling guard (
connections.ts:2531) leaves all 50 verify tests passing. So does removing just the already-held check (:2518). Only removing both fails a test. The test atarcade-verify.test.ts:1837, which is meant to cover already-held, actually passes through the sibling guard: the defaultauthProviderAnswerof"google"turns the earlier rows into siblings. This is the Shared-app protection you called out, so each guard needs its own test. -
Add a regression test for the rolling-deploy fix. Nothing inserts an
mcp_toolsrow withoutvendor_nameand reads it back through a reader, so a future rawvendorNameread would pass CI. One test that does what an old replica does would cover it. -
Show skipped tool names where the admin refreshes.
$key.tsx:1038and$key_.bots.$agentId.tsx:421only showlastErrorwhen the app has zero tools. The refresh mutation also drops the response, so after a partial refresh the admin sees nothing, even though the comment atmodel.ts:637says the names are reported on the app's page. Meanwhileadmin/plugins/index.tsx:184turns the whole row red. For a server that keeps listing a dotted name, that never clears. Show the skipped names as a warning on the app's page, and don't render a partial refresh as an error on the list.
Should fix
- A cancelled action can stay live. When a cancel takes the already-held path, the revoke is skipped and the person is told "cancelled", but the extra scope stays on the Arcade token. Calls are gated only by Arcade's
tool_authorization_required, so the Bot can still do the action they cancelled. On a Shared app, it does so as the team. This is rare, but two things should change:- the notice at
connected-accounts/index.tsx:155should say the action may still be allowed, and how to remove it; - the trail should record more than
authorization_cancelled.
- the notice at
- A probe failure after a cancel logs nothing (
connections.ts:2510). Write thearcade-cancelled-grant-left-liveline there too. - Don't baseline this PR's own type errors. 25 of the 267 baseline entries are in 10 test files this PR adds. That includes 14 copies of one TS2322 in
routes-vendor-vocabulary.test.tsand 8 across thearcade-*-enginetests. The baseline's own header says new code shouldn't add lines, so fix them and drop the entries.
Small
- A dev database that ran the old 0057 to 0063 gets a raw
42P07 relation "pending_authorizations" already existsinstead of the guard's message. Nothing is corrupted. A line in the description is enough, or the guard could refuse when applied migrations are missing from the folder. - The description's "Not done" section still says the admin page offers a whole-app Connect that always fails.
9c441b7dalready hides that button. - The comments at
broker.ts:552andarcade-adapter.ts:233/852/1080still say "Measured against the live API", without the source and date the docs now give. - Optional, since main already behaves this way:
OFFERABLE_TOOL_NAMEhas no length limit. With a 40-character server id,mcp__<serverId>__<name>can pass OpenAI's 64-character limit.
Once 1 to 4 land, I'm happy to approve.
…ncelled grant stays live - accountRow matched a deployment row on provider, app and holder alone, so a withdrawal aimed at a retired Shared identity deleted the CURRENT identity's row (Shared A -> Personal -> Shared B, B connects, the admin returns). It now matches vendor_user_id too. - The already-held and sibling skip-revoke guards each get their own test; the already-held case no longer passes through the sibling guard. - A cancel whose grant is left live answers its own code, cancelled-left-live, telling the person the action may still be allowed and how to remove it, and files mcp.cancelled_grant_left_live beside mcp.authorization_cancelled. - A probe failure after a cancel now writes the arcade-cancelled-grant-left-live line. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…r_name Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…pp's last error The names now go in mcp_servers.skipped_tools (0059, nullable) instead of last_error, so a partial refresh no longer turns the app's row red on the Plugins list for good, and the app page and the Bot's grant page name them whether or not the app has other tools. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
26 entries out of the baseline (267 -> 241): the 25 in test files this PR adds, plus plugin-routes' ServerRecord fixture, now complete. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…heck, and two moved ratchets - gate-foreign-provenance-call: main (#798) reworded the not-connected refusal. - shared-brokered-store: a re-check of ANOTHER identity no longer stamps its verdict on the deployment row, now that accountRow matches the identity. - fixture-identity-distinct: arcade-verify seeds one more person row the way production writes it (5 -> 6). - assertion-shape: two allow-listed lines moved down with the edits above them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
main (#798) added HeldConnection.revoked; this PR's test type gate caught the fixture without it after the merge. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
All of it is in, up to Must fix
Should fix
Small
|
The warning had a test for what it says, but none that either admin page draws it, and none that a clean refresh clears it. Removing the notice from the app page or the Bot's grant page, or keeping the old names on a clean refresh, now fails a test.
… chain The assertion-shape guard refuses a ?. subject under a matcher undefined satisfies, since it passes when nothing was created at all. The body is parsed from JSON, so an absent auth is the same claim, and not.toHaveProperty fails on a missing create.
anthropic 2.0.0, released 2026-10-21, fails every Anthropic run in this harness with Cannot instantiate typing.Union. The same pin is in #814.
Adds Arcade as a second connected-app provider beside Composio. One provider is active per deployment. Four changes are visible to Composio deployments too; see Changes Composio deployments will see below.
Changes Composio deployments will see
COMPOSIO_API_KEYandARCADE_API_KEYset with noOPENBOT_BROKER_PROVIDER: the server now refuses to start, with a message naming the variables. Before, the Composio key alone decided. SetOPENBOT_BROKER_PROVIDER=composioor unset the Arcade key.VENDOR_CALL_TIMEOUT_MS), which a cold cache can hit; the page then shows a timeout instead of hanging.mcp_servers.account_mode = 'personal'on brokered apps (Composio included) that hold an account and have it null. Rows that already saysharedare untouched.mcp_servers.last_error, which turned the app's row red on the Plugins list for good. The names now go in a new nullable column,skipped_tools(migration 0059), and the app's page names them.What this gives a deployment
Arcade authorizes per action, so there is no Connect button. A person authorizes
Gmail.ListEmails; OpenBot mints a consent for exactly that, binds the return leg to the attempt it started, and writes one connection row and one audit event — or writes nothing at all.Two facts are measured and shown before anybody grants anything:
The headline the measurements took away
An earlier draft said "pick Arcade when a security team has to sign off." That is false as a general claim, and the numbers say so:
Gmail)gmail.readonlyvsgmail.sendOn GitHub, consenting to
Github.CountStargazers— a read — flipped all 43 actions to authorized, writes and the destructive one included. Least privilege is a property of (provider × how its OAuth app was registered), not of Arcade. No screen, doc or message in this PR claims otherwise; a test asserts the absence of a confinement promise across every sentence shape.Shape
grantIsLocal— Composio's grant is held here, Arcade's is not, so an Arcade call goes out and the vendor's own refusal is the answer.store.tsrecords a defect where a gate and its exemption keyed on different things and failed open; aprovider === "arcade"comparison inside a permission check is that same shape.confirm_user'sauth_idmust equal the one the mint recorded. Identity alone attributes a late read consent to a fresh send request.auth.statuscarries a live access token. The adapter returns{ status, userId, scopes }and drops the rest inside the adapter — a narrowed return type rather than a rule, because the throwaway harness that proved this flow wrote the token to a file on its first run.A live defect this found and fixed
connectionsForcompared the vendor's auth provider to the toolkit name. Those coincide only when an operator names the provider after the toolkit — whichGithub,AsanaandAttioall do, and which every fixture in the tree uses. For any Google app, the filter matched nothing:isConnectedfalse for a connected account,revokeanswering "nothing there" instead of deleting,accountNamenull. Silent, and at the moment somebody presses Disconnect.Fixed by putting
authProviderOfon the seam and reading it from the listing the adapter already performs.Verification
bun test server/testsas a whole no longer runs: ~100 files open a module-scope pool against PostgreSQL's 100max_connections, so it dies withsorry, too many clients alreadybefore reporting anything. That is a harness ceiling, never a regression. Run it sharded.Not done
server/tests/arcade-live.test.tsran once by hand on 2026-10-13: 9 passed and 3 failed, and all 3 failures were the suite's own mistakes, since fixed. It has not been re-run since. The app was also run locally against a live Arcade project, which found three defects, all now fixed. The last was the admin page offering a whole-app Connect under Arcade that always failed;9c441b7dhides it. CI never dials Arcade.docs/plugins/arcade.mdnow says which of its facts came from which run.42P07 relation "pending_authorizations" already existsinstead of the migration guard's message. Nothing is corrupted. Recreate that dev database and migrate again.OFFERABLE_TOOL_NAMEhas no length limit, so with a long server idmcp__<serverId>__<name>can go over OpenAI's 64-character limit. Main already behaves this way, so it is left for a follow-up.Operator setup is in
docs/plugins/arcade.md.🤖 Generated with Claude Code